Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Describe the process of writing integration tests for GDScript #4842

Merged
merged 1 commit into from
Jun 2, 2021
Merged

Describe the process of writing integration tests for GDScript #4842

merged 1 commit into from
Jun 2, 2021

Conversation

Xrayez
Copy link
Contributor

@Xrayez Xrayez commented Apr 16, 2021

@Calinou Calinou requested a review from vnen April 19, 2021 17:14

./bin/<godot_binary> --gdscript-generate-tests godot-source/modules/gdscript/tests/scripts

4. Run GDScript tests with:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the not to check if the *.out files are expected should come before this step. If you generate the outputs and immediately run the test, it will always pass.

It should be clear also that if generating change other tests that you didn't add, you should revert it back and not commit.

Copy link
Contributor Author

@Xrayez Xrayez May 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I understand correctly. Is it possible to run tests for new scripts that don't have *.out files generated for them yet? Not sure what would be the point of that, since there will be nothing to compare the output against.

I agree that it can be clarified that those generated out files which fail should not be committed, but a regression should reported (yet this likely won't be an issue, since regressions are going to be caught early at CI checks stages). This is also not a big deal for someone who participates in implementing GDScript language itself.

But current version does tell:

Make sure the output does have the expected values before submitting a pull request.

Copy link
Contributor Author

@Xrayez Xrayez May 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is that, the process describes how to write new tests, not updating existing ones. If you believe it's beneficial to describe those processes separately, then it may be worth it. But I'm afraid this could potentially make the documentation more confusing.

Calinou went through those steps successfully while making godotengine/godot#48029, for instance. If someone is going to have a problem, the documentation could be further clarified I guess.

That said, not sure what kind of changes to make without breaking the currently described workflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added an explicit warning:

image

@vnen
Copy link
Member

vnen commented May 3, 2021

In general looks good to me, apart for that small comment.

@akien-mga akien-mga merged commit ce2da75 into godotengine:master Jun 2, 2021
@akien-mga
Copy link
Member

Thanks!

@Xrayez Xrayez deleted the doctest-gdscript branch June 2, 2021 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add integration tests for GDScript
4 participants